Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AutoinstallError exception #1897

Conversation

Chris-Peterson444
Copy link
Contributor

Adds a special exception for Autoinstall related failures and a
specific implementation for autoinstall validation failures.

When a user passes incorrect autoinstall data, the generated
bug report will be titled:

"Autoinstall crashed with AutoinstallValidationError"

Where the traceback will start with a hint on the offending
section. For example:

"Malformed autoinstall in 'apt' section"

Information from the original jsonschema ValidationError is
also available in the traceback to point to specific issues
with the provided data.

The section reporting is a little crude in the validation
of the base schema done by SubiquityServer, which can't discern
between the 'interative-sessions' and 'version' keys, but for now
the scope is pretty limited and can be fixed up at a later time.

Move the assignment of autoinstall_default to the relevant block
where it's being tested.
Provided autoinstall data is already validated against a specific
schema by the relevant controller, but these schemas themselves
are never validated against a meta schema to guarantee they are
valid JSON schemas. This patch adds unit tests for each controller,
as well as SubiquityServer, to validate their autoinstall schema
against the latest JSON meta schema draft (that is supported in
jsonschema). A particular draft validator can be used instead by
specifying meta schema in the schema definition.
@Chris-Peterson444 Chris-Peterson444 changed the title Autoinstall exception fr 6293 AutoinstallError exception Jan 29, 2024
@Chris-Peterson444
Copy link
Contributor Author

Commit f6a7819 is a little big and I'm tempted to create a separate PR for it. Although it's mostly repeated lines to just test the the json schemas defined by the controllers are valid.

Testing this is generally important, but jsonschema.validate can throw an error from either non-conforming data or bad json schemas. We should catch the latter at test time so only validation errors happen at run time.

@Chris-Peterson444
Copy link
Contributor Author

Chris-Peterson444 commented Jan 29, 2024

I updated the commit adding interactive-sections to the base schema to also include it in the documentation's schema

SubiquityServer is responsible for checking and loading the
interactive-sessions, so it should be responsible for validating
this section as well. Additionally add this to the autoinstall
schema in the docs.
@Chris-Peterson444
Copy link
Contributor Author

Rebased to not conflict with #1898

subiquity/server/autoinstall.py Show resolved Hide resolved
subiquity/server/autoinstall.py Outdated Show resolved Hide resolved
subiquity/server/controller.py Outdated Show resolved Hide resolved
Adds a base exception type for Autoinstall related failures and a
specific implementation for autoinstall validation failures.

When a user passes incorrect autoinstall data, the installer will crash
with an AutoinstallValidationError exception. Failure messages are
currently in the form of:

    "Malformed autoinstall in '<autoinstall_key>' section"

where <autoinstall_key> is the name of the top-level key a particular
controller is responsible (e.g., 'apt' and MirrorController).

The section reporting is a little crude in the validation
of the base schema done by SubiquityServer, which can't discern
between the 'interative-sessions' and 'version' keys, but for now
the scope is pretty limited and can be fixed up at a later time.
@Chris-Peterson444
Copy link
Contributor Author

I simplified the new exception types a bit and added handling to explicitly not generate an apport report when failing on an instance of AutoinstallError.

Copy link
Collaborator

@dbungert dbungert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One item to review then I think this is ready.

subiquity/server/server.py Outdated Show resolved Hide resolved
Autoinstall related failures are more likely than not going to be
user caused, so we shouldn't immediately generate a crash report
for these types of failures. This should hopefully allow the user
to debug their autoinstall data much faster and reduce the number
of autoinstall-related bugs reported.
@Chris-Peterson444 Chris-Peterson444 merged commit 48e5f9c into canonical:main Feb 6, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants